Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed and enabled readability-avoid-const-params-in-decls clang-tidy warnings #4861

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Mar 6, 2023

No description provided.

@firewave
Copy link
Collaborator Author

firewave commented Mar 6, 2023

I filed https://trac.cppcheck.net/ticket/11602 for the symbolDatabaseWarning selfcheck warnings.

These changes also triggered the already fixed https://trac.cppcheck.net/ticket/11535.

@firewave firewave marked this pull request as draft March 6, 2023 10:25
@@ -79,7 +79,7 @@ class CppCheckExecutor : public ErrorLogger {
/** xml output of errors */
void reportErr(const ErrorMessage &msg) override;

void reportProgress(const std::string &filename, const char stage[], const std::size_t value) override;
void reportProgress(const std::string &filename, const char stage[], std::size_t value) override;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a very strong opinion. But I don't see how it would hurt to keep these const.

Copy link
Collaborator Author

@firewave firewave Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well they have no effect and we also don't have the definition and declaration in sync. In some cases it might even be misleading. It also exposes bugs in our parsing as we are not properly handling this code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is to have const only in the implementation (where it matters), not in the declaration, right? So we don't actually lose anything here, since top-level const has no effect in a function declaration.

Copy link
Owner

@danmar danmar Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it has no effect on the function interface. but it does have an effect in the function body.

The point is to have const only in the implementation (where it matters), not in the declaration, right?

In my opinion it would be a bad idea to have different const-qualifiers in the declaration and definition/implementation. Right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has no effect. there's also cases where the header and source were not in sync and nothing will tell you. It actually doesn't matter at all. It just makes things "cleaner" - and exposes Cppcheck bugs :)

Copy link
Owner

@danmar danmar Mar 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has no effect.

Well.. it does have an effect in the function body.

void foo(const int x) {
    x=3; // <- not allowed because of const
} 

I do not have a strong opinion if we should use const for parameters passed by value. But in my opinion we should not have different constness in the declaration and implementation. If the implementation uses const then I feel the declaration should too.

And as I said.. how does it hurt with const?

there's also cases where the header and source were not in sync and nothing will tell you.

Are you sure that this is explicitly permitted by the language standard? Can it be a compiler bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well.. it does have an effect in the function body.

That wasn't changed.

Are you sure that this is explicitly permitted by the language standard? Can it be a compiler bug?

Yes, it is allowed. No, it is not a bug. That's what this check is about: https://clang.llvm.org/extra/clang-tidy/checks/readability/avoid-const-params-in-decls.html

The following are the same as the top-level const has no effect:

void f(const int);
void f(int);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well.. I guess the language standard permits it then but I see no reference..

imho.. having different const in the declaration and implementation seems weird to me. And nobody seems to disagree.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how it hurts to have the const. I do find it strange that people intentionally WANT to have different constness in the declaration and implementation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long time lurker.

Interesting discussion in this stackoverflow question.

"Dropping const from function prototypes has the advantage that you don't need to alter the header file if you decide to drop const from implementation part later."

and

"It is all a matter of who needs which information. You provide the parameter by value so the caller does not need to know anything on what you do (internally) with it. ... In your implementation you do care that you can promise not to alter ..."

@firewave firewave marked this pull request as ready for review March 10, 2023 08:27
@firewave firewave force-pushed the tidy-const branch 2 times, most recently from 75a253e to 6f0b83e Compare April 8, 2023 10:57
@firewave firewave force-pushed the tidy-const branch 2 times, most recently from c7c8a9d to 1d2d36c Compare August 8, 2023 23:00
@danmar
Copy link
Owner

danmar commented Aug 22, 2023

conflict..

and I still feel a bit skeptic to have different constness in declaration/implementation. I don't see matching constness as bad style.

@firewave firewave marked this pull request as draft August 7, 2024 18:48
@firewave firewave force-pushed the tidy-const branch 2 times, most recently from d0d7443 to 0bbf502 Compare August 7, 2024 19:37
@firewave firewave force-pushed the tidy-const branch 2 times, most recently from c891783 to dbc885e Compare August 8, 2024 01:21
@firewave firewave marked this pull request as ready for review August 8, 2024 01:21
@firewave firewave merged commit 601156b into danmar:main Aug 16, 2024
63 checks passed
@firewave firewave deleted the tidy-const branch August 16, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants